Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine fanout timeline #12507

Merged
merged 20 commits into from
Dec 2, 2023
Merged

Conversation

anatawa12
Copy link
Member

@anatawa12 anatawa12 commented Nov 29, 2023

What

Why

#12160 はMissRiricaで問題なようなので早めに対応したいなと思いました

Additional info (optional)

TL取得のためのDBアクセス回数が平均して2回になってるとおもいます。

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@github-actions github-actions bot added the packages/backend Server side specific issue/PR label Nov 29, 2023
Copy link
Contributor

github-actions bot commented Nov 29, 2023

このPRによるapi.jsonの差分

差分はこちら
--- base
+++ head
@@ -17743,6 +17743,10 @@
                   },
                   "untilDate": {
                     "type": "integer"
+                  },
+                  "allowPartial": {
+                    "type": "boolean",
+                    "default": false
                   }
                 },
                 "required": [
@@ -46174,6 +46178,10 @@
                   "untilDate": {
                     "type": "integer"
                   },
+                  "allowPartial": {
+                    "type": "boolean",
+                    "default": false
+                  },
                   "includeMyRenotes": {
                     "type": "boolean",
                     "default": true
@@ -46382,6 +46390,10 @@
                     "type": "string",
                     "format": "misskey:id"
                   },
+                  "allowPartial": {
+                    "type": "boolean",
+                    "default": false
+                  },
                   "sinceDate": {
                     "type": "integer"
                   },
@@ -49271,6 +49283,10 @@
                   "untilDate": {
                     "type": "integer"
                   },
+                  "allowPartial": {
+                    "type": "boolean",
+                    "default": false
+                  },
                   "includeMyRenotes": {
                     "type": "boolean",
                     "default": true
@@ -49825,6 +49841,10 @@
                   "untilDate": {
                     "type": "integer"
                   },
+                  "allowPartial": {
+                    "type": "boolean",
+                    "default": false
+                  },
                   "includeMyRenotes": {
                     "type": "boolean",
                     "default": true
@@ -59900,6 +59920,10 @@
                   "untilDate": {
                     "type": "integer"
                   },
+                  "allowPartial": {
+                    "type": "boolean",
+                    "default": false
+                  },
                   "withFiles": {
                     "type": "boolean",
                     "default": false

Get diff files from Workflow Page

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Attention: 100 lines in your changes are missing coverage. Please review.

Comparison is base (cf3d45e) 78.65% compared to head (9cd8ccb) 78.74%.

Files Patch % Lines
...kend/src/server/api/endpoints/channels/timeline.ts 11.11% 48 Missing ⚠️
...es/backend/src/server/api/endpoints/users/notes.ts 81.89% 21 Missing ⚠️
...d/src/server/api/endpoints/notes/local-timeline.ts 75.67% 9 Missing ⚠️
.../src/server/api/endpoints/notes/hybrid-timeline.ts 83.72% 7 Missing ⚠️
...backend/src/server/api/endpoints/notes/timeline.ts 81.08% 7 Missing ⚠️
...c/server/api/endpoints/notes/user-list-timeline.ts 84.37% 5 Missing ⚠️
.../backend/src/core/FanoutTimelineEndpointService.ts 97.56% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12507      +/-   ##
===========================================
+ Coverage    78.65%   78.74%   +0.08%     
===========================================
  Files          950      953       +3     
  Lines       103382   103616     +234     
  Branches      8329     8328       -1     
===========================================
+ Hits         81319    81592     +273     
+ Misses       22063    22024      -39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@syuilo
Copy link
Member

syuilo commented Dec 2, 2023

#12254
ここで話されたように allowPartial のようなオプションが false の場合だけ追加の問い合わせを行うようにする方がパフォーマンスの観点からは良さそうに思います(そもそもlimitは最大数の指定であって、指定された数きっちり返すというcountの意味ではないというのもある)

@anatawa12
Copy link
Member Author

了解です。allowPartial追加します。

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Dec 2, 2023
@anatawa12
Copy link
Member Author

notes/timeline のドキュメント見ても少なくなる可能性があるとは書かれておらず、実際にlimitまでレスポンスがあることに依存したクライアントが存在するため、互換性のためallowPartialのデフォルトはfalseにしています。

@syuilo
Copy link
Member

syuilo commented Dec 2, 2023

デフォルトでfalseだと、これから実装されるクライアントなどについてわざわざtrueでリクエストしてくることは少なそうだから効果が限定的になりそうという懸念があるわね
ユーザーからしても、最初読み込んだタイムラインに含まれる投稿が20でも19でも気にしないと思うから、基本的にはlimitはlimitであってcountではないという前提でクライアントを実装してほしいから、デフォルトはtrueにして、既存のクライアントでどうしても対応できない場合に allowPartial: false でリクエストするようにしてほしいというのが自分の考え

@syuilo
Copy link
Member

syuilo commented Dec 2, 2023

もしくはv2みたいにエンドポイント分ける手もあるわね

@syuilo
Copy link
Member

syuilo commented Dec 2, 2023

もしくはv2みたいにエンドポイント分ける手もあるわね

タイムラインAPIのレスポンスはオブジェクトではなく配列そのままで、ページネーションに関する情報などを追加できないという問題があって以前からv2としてエンドポイント更新したいと考えてたからそのタイミングでここら辺の挙動も変えるという感じ

@anatawa12
Copy link
Member Author

既存のクライアントを使用してるユーザーはクライアントの更新が入るまで正しく動かない状態で問題ないという考えでしょうか?
pagenationの処理として、帰ってきたnoteの数が0かどうかで最後を見分けるのではなく、リクエストした数帰ってきてるかどうか(足りないかどうか)で判断するのは十分納得できる実装だと思います。

@anatawa12
Copy link
Member Author

v2にする場合パスどのようにしますか?

@syuilo
Copy link
Member

syuilo commented Dec 2, 2023

v2にする場合パスどのようにしますか?

v2追加するのはもっと大きなトピックになるからここでは詳しい議論はしないけど、/api/v2/notes/timeline とかになりそう

@syuilo
Copy link
Member

syuilo commented Dec 2, 2023

テストが通れば良さそう

@anatawa12
Copy link
Member Author

なんでさっき通らなかったんだろ...
通りました

@syuilo syuilo merged commit a631b97 into misskey-dev:develop Dec 2, 2023
18 checks passed
@syuilo
Copy link
Member

syuilo commented Dec 2, 2023

🙏🏻 🙏🏻 🙏🏻

@anatawa12 anatawa12 deleted the refine-fanout-timeline branch December 2, 2023 09:29
@samunohito samunohito mentioned this pull request Dec 2, 2023
5 tasks
camilla-ett pushed a commit to kaseiski/misskey that referenced this pull request Jan 2, 2024
* chore(endpoints/hybrid-timeline): don't pack inside getFromDb

* chore(endpoints/hybrid-timeline): Redisから取得する部分のうちSTLに依存しなそうなところを別のServiceに切り出し

* chore(endpoints/local-timeline): FanoutTimelineEndpointServiceで再実装

* chore(endpoints/channels/timeline): FanoutTimelineEndpointServiceで再実装

* chore(endpoints/timeline): FanoutTimelineEndpointServiceで再実装

* chore(endpoints/user-list-timeline): FanoutTimelineEndpointServiceで再実装

* chore(endpoints/users/notes): FanoutTimelineEndpointServiceで再実装

* chore: add useDbFallback to FanoutTimelineEndpointService.timeline and always true for channel / user note list

* style: fix lint error

* chore: split logic to multiple functions

* chore: implement redis fallback

* chore: 成功率を上げる

* fix: db fallback not working

* feat: allowPartial

* chore(frontend): set allowPartial

* chore(backend): remove fallbackIfEmpty

HTL will never be purged so it's no longer required

* fix: missing allowPartial in channel timeline

* fix: type of timelineConfig in hybrid-timeline

---------

Co-authored-by: syuilo <Syuilotan@yahoo.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend Server side specific issue/PR packages/frontend Client side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[2023.10.x~]ノート取得時にノート数がlimitよりも少ない事がある
2 participants